Update the types of dataclass attributes according to usage #163

kvid · 2020-09-01T16:18:53Z

Fixes #156

kvid

A couple of my comments below are connected to the wrong code line because Github didn't allow commenting code lines that are not yet changed in this PR. Use https://github.com/formatc1702/WireViz/pull/163/files to expand the hidden code lines.
Are we using type annotations only for dataclass attributes, or should we use it for the rest of the code as well?

src/wireviz/DataClasses.py

kvid · 2020-09-13T00:26:27Z

src/wireviz/DataClasses.py

+Name = str # Case insensitive unique name of connector or cable
+Pin = Union[int, str] # Pin identifier
+Wire = Union[int, str] # Wire number or 's' for shield
+MLstr = str # Multi-line string where any newline is properly handled


If PR #164 is accepted, then we might need 3 different text types:

Text = Text that might contain HTML hyperlinks that are removed in all outputs except in HTML output.

TextML = Text that might contain HTML hyperlinks that are removed in all outputs except in HTML output, and newlines that are translated to <br/> in diagram output or to space otherwise.

str = Plain text that doesn't contain any HTML tags nor newline.

formatc1702 · 2020-10-10T10:47:22Z

This is a very nice contribution.
I suggest changing Name to Designator since that is what has been used in the documentation and the BOM as well, and is a bit less generic.
While my initial reaction was skepticism at having too many custom datatypes, I can see the value in explicitly defining them so there are no surprises when HTML links are stripped, etc... because yes, #164 is a great new feature.
Now, my worry is just that Text and TextML are not self-explanatory enough (I know you added comments in their definitions)... Maybe something like str_html and str_html_multiline? I'm open for better suggestions

Fixes wireviz#156

formatc1702 · 2020-10-23T07:19:07Z

According to your suggestion, this PR should be the next one to be merged.
If you agree with my suggested naming changes, feel free to implement them and rebase onto dev :)
If you feel there is a need for additional discussion, fire away!

kvid · 2020-10-27T16:52:44Z

I'm sorry for this late reply.

This is a very nice contribution.

Thank's.

I suggest changing Name to Designator since that is what has been used in the documentation and the BOM as well, and is a bit less generic.

I agree.

While my initial reaction was skepticism at having too many custom datatypes, I can see the value in explicitly defining them so there are no surprises when HTML links are stripped, etc... because yes, #164 is a great new feature.
Now, my worry is just that Text and TextML are not self-explanatory enough (I know you added comments in their definitions)... Maybe something like str_html and str_html_multiline? I'm open for better suggestions

PEP 484 recommends capitalizing alias names, since they represent user-defined types, which (like user-defined classes) are typically spelled that way.

I therefore suggest these alternatives:

PlainText = str # Text not containing HTML tags nor newlines
LinkText = str # Text possibly including HTML hyperlinks that are removed in all outputs except HTML output
MultilineLinkText = str # LinkText possibly also including newlines to break lines in diagram output

formatc1702 · 2020-10-29T15:45:18Z

I'm sorry I keep nagging about the exact name of the user-defined types.

IMHO, LinkText creates the expectation that this is a text that always/usually is or includes a link.
However, as I mentioned in my comment in #168:

TBH, having a separate url field will probably make the need for including URLs in the type attribute obsolete in most cases.

That's why I am uncomfortable pushing what I expect to be an edge case use for an attribute, into the very definition of that attribute.
My only counter-suggestion, however, might be RichText/RichTextMultiline or String_HTML, which on the other hand creates an expectation that it supports much more than just links.

Another option is getting rid of the custom types, marking everything as str and adding a little comment in the dataclass mentioning the fact that a certain attribute has the custom Link/Linebreak behavior. Personally, I am leaning towards this option again after all...

Since I don't want this problem delaying integration much further, I will merge it as-is tomorrow unless we find a nicer alternative (or unless you want to change something else before, let me know!). We can always rename the type later if something better pops up.

kvid · 2020-10-29T18:13:41Z

I'm sorry I keep nagging about the exact name of the user-defined types.

Extra effort to find good identifier names should be an investment that hopefully pays off in the future by reducing the number of misunderstandings by users.

IMHO, LinkText creates the expectation that this is a text that always/usually is or includes a link.

I see your point. Is TextThatMightContainLinks or TextWithOptionalLinks any better?

However, as I mentioned in my comment in #168:

TBH, having a separate url field will probably make the need for including URLs in the type attribute obsolete in most cases.

I don't fully agree. Supporting link tags is way more flexible in the sense that the user can visualize what the link represents by selecting only a section of the text attribute as the clickable link text, and if needed, link to different URLs from different text sections.

That's why I am uncomfortable pushing what I expect to be an edge case use for an attribute, into the very definition of that attribute.

The whole point of type hints is to document what kind of values that are valid, so the type alias names should describe the differences somehow.

My only counter-suggestion, however, might be RichText/RichTextMultiline or String_HTML, which on the other hand creates an expectation that it supports much more than just links.

A better alternative is perhaps Hypertext that is defined by Wikipedia as text displayed on a computer display or other electronic devices with references (hyperlinks) to other text that the reader can immediately access.

If you like RichText better, it can be explained as text that might contain tags for the HTML output, but currently only supporting link tags, and add support for more tags later on. However, I believe the Rich Text term is more used to describe other formats and markup languages that HTML, e.g. RTF.

Another option is getting rid of the custom types, marking everything as str and adding a little comment in the dataclass mentioning the fact that a certain attribute has the custom Link/Linebreak behavior. Personally, I am leaning towards this option again after all...

If we are going to use type hints in more parts of the source code than DataClasses.py, then the different type aliases will get more useful to avoid the same comments at several locations.

Since I don't want this problem delaying integration much further, I will merge it as-is tomorrow unless we find a nicer alternative (or unless you want to change something else before, let me know!). We can always rename the type later if something better pops up.

True, but I'm willing to think a bit more before we decide.

formatc1702 · 2020-10-29T18:31:37Z

Hypertext! That's a good one!

It almost sounds a bit retro (in a good way), but per the definition you posted, it's pretty close to what we are trying to communicate through the type hint, without over-promising (like RichText).

I wouldn't feel bad including Hypertext and HypertextMultiline, thanks for the cool suggestion!

kvid · 2020-10-29T21:16:07Z

Hypertext! That's a good one!

Then we can agree on that one. 😃

It almost sounds a bit retro (in a good way), but per the definition you posted, it's pretty close to what we are trying to communicate through the type hint, without over-promising (like RichText).

The term is from 1963 and has been used in different projects and predecessors of HTML/HTTP. The term is also the leading and major part of both HTML and HTTP.

I wouldn't feel bad including Hypertext and HypertextMultiline, thanks for the cool suggestion!

Personally, I feel MultilineHypertext is easier to read as it has the natural order of these words in an English sentence. Do you have strong feelings or arguments for the opposite order?

formatc1702 · 2020-10-30T07:14:24Z

It almost sounds a bit retro (in a good way), but per the definition you posted, it's pretty close to what we are trying to communicate through the type hint, without over-promising (like RichText).

The term is from 1963 and has been used in different projects and predecessors of HTML/HTTP. The term is also the leading and major part of both HTML and HTTP.

Of course it's still relevant; I just haven't heard anybody use the actual term "hypertext" in conversation or in writing for a veeery long time ;-)

Personally, I feel MultilineHypertext is easier to read as it has the natural order of these words in an English sentence. Do you have strong feelings or arguments for the opposite order?

My main argument would be that, in certain situations, it makes sense to assign names starting from the general, then going into the specific. In this particular case, it's not really that important, so MultilineHypertext is fine too!

formatc1702 · 2020-10-31T16:29:44Z

Is this ready for merging from your perspective? I saw you force-pushed the latest changes, but I'm not sure.

I would like to try something out: Once you are done with your changes, please use the link in the top right of this PR to request a review from me. That lets me know you are finished with your changes, and if I approve the review, I can merge without having to ask again. Thanks!

Using Any or str in type annotations might increase the need for extra comments to explain the real valid values. However, such needs can be drastically reduced with the help of semanticly named type aliases. Each type alias have their legal values described in comments. Actual validation might be implemented in the future.

kvid · 2020-11-01T03:37:25Z

Is this ready for merging from your perspective? I saw you force-pushed the latest changes, but I'm not sure.

Yes, now I think it should be ready. I squashed most commits into one, but kept the first commit separate to avoid hiding this separate issue in all the type alias changes afterwords.

I would like to try something out: Once you are done with your changes, please use the link in the top right of this PR to request a review from me. That lets me know you are finished with your changes, and if I approve the review, I can merge without having to ask again. Thanks!

I would like to test this feature, but I can't find the link you describe. I found this old blog posting about it, but I cannot find the cog wheel icon described there. This how my right column looks like:

formatc1702 · 2020-11-01T11:21:05Z

src/wireviz/DataClasses.py

+    manufacturer: Optional[MultilineHypertext] = None
+    mpn: Optional[MultilineHypertext] = None
+    pn: Optional[Hypertext] = None


Maybe the answer to my question is buried in the discussion on #115, but:

Why are manufacturer and mpn multiline-capable, and pn is not?

Personally, I'm not sure why any of them would need to be multiline, but at least it should be consistend, since all of them are passed through the html_line_breaks() function.

Maybe the answer to my question is buried in the discussion on #115, but:

This feature is much older that #115.

Why are manufacturer and mpn multiline-capable, and pn is not?

Blame tells me the change was committed by you in 102c7d6 as suggested by me in #136 (comment): In case a user need a long manufacturer info, I suggest supporting line breaks to avoid a very wide node. And since both manufacturer and mpn was placed in the same table cell, I suggested calling html_line_breaks on the whole cell contents.

Personally, I'm not sure why any of them would need to be multiline, but at least it should be consistend, since all of them are passed through the html_line_breaks() function.

Currently, (since the commit mentioned above that is included in v0.2) manufacturer and mpn are passed through the html_line_breaks() function, but not pn, and that's why I used different type hints to reflect how it is currently implemented.

I understand your argument about consistency, but then we also need to change the implementation. I have an idea about moving html_line_breaks() in #168 that will solve your consistency request, but it's still WIP.

Thanks for digging through history for me :)

OK, then let's keep it as-is to reflect the current implementation, and I'll wait for #168.

formatc1702 · 2020-11-01T11:22:41Z

According to the GitHub documentation:

Pull request authors can't request reviews unless they are either a repository owner or collaborator with write access to the repository.

This doesn't make sense to me, but I guess it's how things work. So for now, the only option I see is for the PR author to post an explicit comment stating that the code is ready for [re-]review :/

The alternative would be for me (the owner+reviewer) to set the PR back to draft status if I request changes, and for you (the PR author) to remove draft status again to signal readiness for review; but that doesn't seem like the right way to use the draft feature.

kvid commented Sep 5, 2020

View reviewed changes

kvid force-pushed the issue156-types branch from fbcae68 to 69b657d Compare September 9, 2020 09:21

kvid commented Sep 13, 2020

View reviewed changes

kvid mentioned this pull request Sep 13, 2020

Syntax documentation + new /docs directory #111

Merged

This was referenced Oct 4, 2020

[meta] Collaborator discussion #101

Open

add length_unit support for wire and cable lengths #171

Closed

formatc1702 added this to the v0.3 milestone Oct 17, 2020

Update the types of dataclass attributes according to usage

a050e9d

Fixes wireviz#156

formatc1702 mentioned this pull request Oct 23, 2020

Add href attribute to connectors, cables, and additional_bom_items #168

Draft

kvid force-pushed the issue156-types branch from 69b657d to 35def35 Compare October 27, 2020 16:53

kvid force-pushed the issue156-types branch from 35def35 to 1fdd5ab Compare October 30, 2020 19:28

kvid force-pushed the issue156-types branch from 1fdd5ab to c04804c Compare November 1, 2020 02:51

formatc1702 reviewed Nov 1, 2020

View reviewed changes

formatc1702 merged commit 64bd34a into wireviz:dev Nov 1, 2020

formatc1702 added a commit that referenced this pull request Nov 1, 2020

Add #156, #163

2f362e6

formatc1702 mentioned this pull request Nov 1, 2020

[internal] Using the Optional[] keyword in dataclasses #156

Closed

kvid deleted the issue156-types branch March 7, 2021 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the types of dataclass attributes according to usage #163

Update the types of dataclass attributes according to usage #163

kvid commented Sep 1, 2020

kvid left a comment •

edited

Loading

kvid Sep 13, 2020 •

edited

Loading

formatc1702 commented Oct 10, 2020 •

edited

Loading

formatc1702 commented Oct 23, 2020

kvid commented Oct 27, 2020

formatc1702 commented Oct 29, 2020 •

edited

Loading

kvid commented Oct 29, 2020 •

edited

Loading

formatc1702 commented Oct 29, 2020 •

edited

Loading

kvid commented Oct 29, 2020

formatc1702 commented Oct 30, 2020

formatc1702 commented Oct 31, 2020

kvid commented Nov 1, 2020

formatc1702 Nov 1, 2020

kvid Nov 1, 2020

formatc1702 Nov 1, 2020

formatc1702 commented Nov 1, 2020 •

edited

Loading

Update the types of dataclass attributes according to usage #163

Update the types of dataclass attributes according to usage #163

Conversation

kvid commented Sep 1, 2020

kvid left a comment • edited Loading

Choose a reason for hiding this comment

kvid Sep 13, 2020 • edited Loading

Choose a reason for hiding this comment

formatc1702 commented Oct 10, 2020 • edited Loading

formatc1702 commented Oct 23, 2020

kvid commented Oct 27, 2020

formatc1702 commented Oct 29, 2020 • edited Loading

kvid commented Oct 29, 2020 • edited Loading

formatc1702 commented Oct 29, 2020 • edited Loading

kvid commented Oct 29, 2020

formatc1702 commented Oct 30, 2020

formatc1702 commented Oct 31, 2020

kvid commented Nov 1, 2020

formatc1702 Nov 1, 2020

Choose a reason for hiding this comment

kvid Nov 1, 2020

Choose a reason for hiding this comment

formatc1702 Nov 1, 2020

Choose a reason for hiding this comment

formatc1702 commented Nov 1, 2020 • edited Loading

kvid left a comment •

edited

Loading

kvid Sep 13, 2020 •

edited

Loading

formatc1702 commented Oct 10, 2020 •

edited

Loading

formatc1702 commented Oct 29, 2020 •

edited

Loading

kvid commented Oct 29, 2020 •

edited

Loading

formatc1702 commented Oct 29, 2020 •

edited

Loading

formatc1702 commented Nov 1, 2020 •

edited

Loading